Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

override previous O shared keys @ AllSelection & rowMode: 'array' processing @ PostgresDriver. #1101

Open
wants to merge 10 commits into
base: v0.28
Choose a base branch
from

Conversation

igalklebanov
Copy link
Member

@igalklebanov igalklebanov commented Aug 4, 2024

Hey 👋

fixes #1094.

Running the following query:

const result = await ctx.db
  .selectFrom('person')
  .leftJoin('toy', 'person.id', 'toy.id')
  .selectAll('person')
  .selectAll('toy')
  .execute()

On the 4 core dialects in our tests,
yields the following values for the shared id column:

pg mysql2 tedious better-sqlite3
1 1 1 1
2 null null null
3 null null null

As you can see, pg's results take person.id values, and not toy.id values. Other dialects do the opposite.

To align pg results with the other dialects, this PR uses rowMode: 'array' in PostgresDriver to get the raw values and produce objects that take the last value of a column with the same name. This is a breaking change for PostgreSQL users who relied on the default behavior of pg.

pg mysql2 tedious better-sqlite3
1 1 1 1
null null null null
null null null null

This PR doesn't handle the .selectAll() AND .selectAll(['person', 'toy']) cases. Here we'll get a union of all possible values of a shared column which is not accurate, but can be $narrowType'd. .selectAll(['person', 'toy']) can be solved with compile-time array recursion (which afaik is not that performant - not worth it??). .selectAll() requires a lot of testing - probably affected by order of introduction of tables to query context - if so, will require a huge refactor to TB[] and basically most types.

We can recommend our users to use multiple single-table calls, as the tightest option.
We can deprecate the multi-table variant and even the empty variant of selectAll.

Should we allow users to switch back to pg's default row processing with a flag in the dialect config?

igalklebanov and others added 8 commits July 19, 2024 13:05
…kysely-org#1085)

* add reusable helpers recipe and implement missing expression features

* force node 22.4.1 in CI because of an npm bug
* feat: add postgres range types (kysely-org#1086)

* feat: support refresh naterialized view

* fix tests by adding .materialized() to remove the matview

* fix failing test

* fix: References typo (kysely-org#1092)

* chore: refresh-view-node.ts => refresh-materialized-view-node.ts

* chore: export node in index.ts

---------

Co-authored-by: Isak <16906089+kansson@users.noreply.github.com>
Co-authored-by: Jonathan Wu <jonathanwu70@gmail.com>
@igalklebanov igalklebanov added bug Something isn't working typescript Related to Typescript labels Aug 4, 2024
Copy link

vercel bot commented Aug 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 5, 2024 1:25am

@igalklebanov igalklebanov added postgres Related to PostgreSQL built-in dialect Related to a built-in dialect breaking change Includes breaking changes labels Aug 5, 2024
@igalklebanov igalklebanov marked this pull request as ready for review August 5, 2024 01:21
@igalklebanov igalklebanov requested a review from koskimas August 5, 2024 01:21
@igalklebanov igalklebanov changed the title minor enhancement to AllSelection that overrides previous O shared keys. override previous O shared keys @ AllSelection & rowMode: array processing @ PostgresDriver. Aug 5, 2024
@igalklebanov igalklebanov changed the title override previous O shared keys @ AllSelection & rowMode: array processing @ PostgresDriver. override previous O shared keys @ AllSelection & rowMode: 'array' processing @ PostgresDriver. Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Includes breaking changes bug Something isn't working built-in dialect Related to a built-in dialect postgres Related to PostgreSQL typescript Related to Typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants